Is the test on this.owner._repeat needed? That is, isn't the test on this.owner sufficient?

At first, I worried the case of using setTimeout but I found that it doesn't matter.
Right, this.owner._repeat is not necessary. Fixed.

This comment seems a bit too specific, even if it is correct. What about:

// Do not close the underlying handle if its ownership has changed (e.g it was unrefed in its callback).

?

Fixed

Nit: I would rephrase this as:

// Checks that setInterval timers keep running even when they're unrefed within their callback.

Fixed

I'm not sure asserting on a specific number of occurrences that the setInterval was called is robust. For instance, it seems that if clearInterval didn't work properly after merging this change but it took less than 1ms to complete one turn of the event loop and thus close the server handle, this test wouldn't fail.

counter2 increments up to 11 before server.close() so that the test is not failed even when server is closed immediately. The test can be finished without need to wait for the next event loop.

Here's what I understand of how the test Test2 works and what its goal is:

• The goal is to make sure that, when calling clearInterval on an unrefed() repeating timer created with setInterval, the timer doesn't fire anymore.

• To test that, we:

  1. create a repeating timer with setInterval
  2. make sure it fires 11 times
  3. clear it
  4. test that it was properly cleared by testing that, when the process exits, the value of counter2 is 11.

What I'm saying is that, even if the timer was not cleared properly, it would be possible for this test to:

• not fire the cleared interval's callback (and thus leave the value of counter2 at 11)
• close the server handle
• let the process exit

if it took less than the time it takes for the interval timer to fire (1ms or more) to close the server and terminate the event loop. In that case, we'd still think the test succeeded, when clearly the implementation would have a bug.

Does that make sense or am I missing something?

@misterdjules I've got it. That can be resolve by swapping Test1 and Test2 for it needs 10 msec after testing clearInterval. Fixed in 589d661cdd8d83e848cea5b09d4dd2cdae2fe365. PTAL.

Would it still work if this.close() was placed into both of these ifcases? I think that may be a better and more robust patch.

It would work as far as I checked. I also made an additional test that has require('timers')._unrefActive(timer1) and found that the timer handle is not reused in that case. So it is okay.
Honestly, I'm not sure whether the following two conditions are equivalent or not. I think the latter is clearly understandable.

  if (list._unrefed === true && list === unrefedLists[msecs]) {
    delete unrefedLists[msecs];
    this.close();
  } else if (list === refedLists[msecs]) {
    delete refedLists[msecs];
    this.close();
  }

  // Do not close the underlying handle if its ownership has changed
  // (e.g it was unrefed in its callback).
  if (this.owner)
    return;

  this.close();

My thinking is that you only actually want to close the handle if the list was in a pool.

There are more obscure parts of the timers code imo so I don't think it is an issue.

@Fishrock123

My thinking is that you only actually want to close the handle if the list was in a pool.

Do you mean that we want to close the TimerWrap handle only if the list is still in a pool?

Does that diff reflects the change you had in mind:

diff --git a/lib/timers.js b/lib/timers.js
index 0784f7f..fb81467 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -160,6 +160,7 @@ function TimersList(msecs, unrefed) {
 }
 
 function listOnTimeout() {
+  var pool;
   var list = this._list;
   var msecs = list.msecs;
 
@@ -222,15 +223,27 @@ function listOnTimeout() {
   // As such, we can remove the list and clean up the TimerWrap C++ handle.
   debug('%d list empty', msecs);
   assert(L.isEmpty(list));
-  this.close();
-
-  // Either refedLists[msecs] or unrefedLists[msecs] may have been removed and
-  // recreated since the reference to `list` was created. Make sure they're
-  // the same instance of the list before destroying.
-  if (list._unrefed === true && list === unrefedLists[msecs]) {
-    delete unrefedLists[msecs];
-  } else if (list === refedLists[msecs]) {
-    delete refedLists[msecs];
+
+  if (list._unrefed) {
+    pool = unrefedLists;
+  } else {
+    pool = refedLists;
+  }
+
+  /*
+   * If the timers list that was just traversed and is empty actually doesn't
+   * exist anymore (e.g because it was reused to create an unrefed timer), don't
+   * close the underlying timerwrap, its ownership changed.
+   */
+  if (pool[msecs] !== undefined) {
+    this.close();
+  }
+
+  // The timers list may have been removed and recreated since the reference to
+  // `list` was created. Make sure they're the same instance of the list before
+  // destroying.
+  if (list === pool[msecs]) {
+    delete pool[msecs];
   }
 }
 

?

I would think that the owner property represents ownership for a specific timer handle better than the belonging of its timer list to a given timer lists pool, so I have a preference for:

// Do not close the underlying handle if its ownership has changed
// (e.g it was unrefed in its callback).
if (this.owner)
  return;

this.close();

I think that if we check the closing condition weather the list was in a pool or not.

+  if (pool[msecs] !== undefined) {
+    this.close();
+  }

is better.
Otherwise, I prefer to check owner property.

I'm very confused as to why what I originally suggested doesn't do the job just fine?

  if (list._unrefed === true && list === unrefedLists[msecs]) {
    delete unrefedLists[msecs];
    this.close();
  } else if (list === refedLists[msecs]) {
    delete refedLists[msecs];
    this.close();
  }

Keep in mind that reuse() is called before this if unrefing is actually re-using the handle and it already removed the list from the pool:

Essentially I don't see why we'd bother to check .owner() if doing it in those conditionals is deterministic anyways?

@Fishrock123

Essentially I don't see why we'd bother to check .owner() if doing it in those conditionals is deterministic anyways?

For the reason I mentioned above:

I would think that the owner property represents ownership for a specific timer handle better than the belonging of its timer list to a given timer lists pool

In other words, it seems it would be more robust to check the owner property than rely on what seems to be an implementation detail.

The solution you're suggesting also duplicates code, and doesn't include any documentation about why the underlying TimerWrap handle should be closed in some cases, and not in others.

What you're suggesting would indeed fix the problem, but it seems to me it would lead to a less robust solution that would be more difficult to understand.

What you're suggesting would indeed fix the problem, but it seems to me it would lead to a less robust solution that would be more difficult to understand.

I agree this.
And it is the point that we don't have an agreement with @Fishrock123 from his comment of

There are more obscure parts of the timers code imo so I don't think it is an issue.

s/also// since it's now the first test in this tests suite.

@shigeki

This test should be executed at first to confirm that timer1 is surly stopped in 10 msec while Test2 is running.

I think this description lacks some details and has some errors that prevent it from being easily understandable by someone not already familiar with this specific tests suite and the timers implementation.

Here's what I'd suggest as a replacement for the whole block of comment:

Test1 checks that clearInterval works as expected for a timer unrefed within its callback: it removes the timer and its callback is not called anymore. Note that the only reason why this test is robust is that:

1. the repeated timer it creates has a delay of 1ms
2. when this test is completed, another test starts that creates a new repeated timer with the same delay (1ms)
3. because of the way timers are implemented in libuv, if two repeated timers A and B are created in that order with the same delay, it is guaranteed that the first occurrence of timer A will fire before the first occurrence of timer B
4. as a result, when the timer created by Test2 fired 11 times, if the timer created by Test1 hadn't been removed by clearInterval, it would have fired 11 more times, and the assertion in the process' exit event handler would fail.

What do you think?

@misterdjules It describes much more details for Test1. There is no reason against for it. Fixed in dffeb3b97915ec3582205dfa883cac0aaf1f9ad4.

clearInterval works in a very specific way (in the end, it sets _idleTimeout = 0) and while it is affected in this problem it is unnecessary for testing it?

I intended to add the clearInterval test because it executes timer.close() and to check if it is not affected by this fix. I think it can prove this fix has no side effects on timer.close() during the use of unrefed setInterval.

You should only need to do 3 intervals for either, regardless. That should be enough to prove it is functioning.

Okay, 10 is no meaning. Fixed to 3 intervals.